ARROW-14708: [C++] Adding missing abseil dependencies to enable static flight build #11889
Conversation
|
|
|
Thanks! Unfortunately I still get a lot of linker errors using the repro from JIRA, but at least the Abseil issues are eliminated. I think the remaining issues are just because of the order of libraries. This is what CMake generates, which fails with linker errors: The errors indicate This works: i.e. link libarrow_flight, then libarrow, then libarrow_bundled_dependencies, then OpenSSL. Flipping the order of libraries in the CMakeLists.txt fixes the first issue, so I suppose this is user error, or we're missing some dependency expressed in CMake(?). The installed ArrowFlightTargets.cmake does not declare arrow_static in INTERFACE_LINK_LIBRARIES - my CMake understanding is poor but perhaps we need to do so? (It is odd because arrow_static is in the STATIC_LINK_LIBS in flight/CMakeLists.txt.) Also, ArrowTargets.cmake declares OpenSSL as part of arrow_static's INTERFACE_LINK_LIBRARIES, but ArrowConfig.cmake does not do the same for arrow_bundled_dependencies. |
|
This diff gets CMake to repeat diff --git a/cpp/src/arrow/flight/CMakeLists.txt b/cpp/src/arrow/flight/CMakeLists.txt
index 8a3228e50..846a420f9 100644
--- a/cpp/src/arrow/flight/CMakeLists.txt
+++ b/cpp/src/arrow/flight/CMakeLists.txt
@@ -170,7 +170,9 @@ add_arrow_lib(arrow_flight
${ARROW_FLIGHT_LINK_LIBS}
STATIC_LINK_LIBS
arrow_static
- ${ARROW_FLIGHT_LINK_LIBS})
+ ${ARROW_FLIGHT_LINK_LIBS}
+ STATIC_INSTALL_INTERFACE_LIBS
+ arrow_static)
foreach(LIB_TARGET ${ARROW_FLIGHT_LIBRARIES})
target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_FLIGHT_EXPORTING)And this additional diff, while questionable, fixes the rest of the link errors: diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
index 6209baeec..750d3aba6 100644
--- a/cpp/src/arrow/ArrowConfig.cmake.in
+++ b/cpp/src/arrow/ArrowConfig.cmake.in
@@ -87,6 +87,10 @@ if(NOT (TARGET arrow_shared OR TARGET arrow_static))
set_target_properties(
arrow_static PROPERTIES INTERFACE_LINK_LIBRARIES
"${arrow_static_interface_link_libraries};arrow_bundled_dependencies")
+ # Propagate dependencies like OpenSSL
+ set_target_properties(
+ arrow_bundled_dependencies PROPERTIES INTERFACE_LINK_LIBRARIES
+ "${arrow_static_interface_link_libraries}")
endif()
endif()
endif() |
|
Thanks @lidavidm I was gonna ask for ideas about the flight linking errors! I'll include your diffs and test. |
|
@lidavidm I'm able to compile this locally now. I've also reduced the abseil dependencies (I think to a minimum). |
|
@github-actions crossbow submit -g nightly |
|
Revision: f3745060a1982f3ee46705bec28df3830312a8b9 Submitted crossbow builds: ursacomputing/crossbow @ actions-1268
|
|
@kou Sorry to trouble you, but would you be able to look over the CMake changes here? I'm not sure if the changes to ArrowConfig.cmake.in are strictly correct. |
cpp/src/arrow/ArrowConfig.cmake.in
Outdated
There was a problem hiding this comment.
I don't know why this is needed.
Is there a use-case that uses arrow_bundled_dependencies without arrow_static?
BTW, indent for this code is broken.
There was a problem hiding this comment.
Ah, the problem here is the order of libraries to the linker. As described above, currently, the order is OpenSSL, then libarrow_bundled_dependencies, so linking will fail because libarrow_bundled_dependencies itself also depends on OpenSSL. Adding this gets CMake to repeat OpenSSL after libarrow_bundled_dependencies and then the linker will find the necessary symbols.
There was a problem hiding this comment.
I see. Then we should list OpenSSL in INTERFACE_LINK_LIBRARIES of arrow_bundled_dependencies not arrow_static and INTERFACE_LINK_LIBRARIES of arrow_static should list only arrow_bundled_dependencies something like this:
diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
index 6209baeec..ac2b9a027 100644
--- a/cpp/src/arrow/ArrowConfig.cmake.in
+++ b/cpp/src/arrow/ArrowConfig.cmake.in
@@ -38,6 +38,7 @@ set(ARROW_LIBRARY_PATH_SUFFIXES "@ARROW_LIBRARY_PATH_SUFFIXES@")
set(ARROW_INCLUDE_PATH_SUFFIXES "@ARROW_INCLUDE_PATH_SUFFIXES@")
set(ARROW_SYSTEM_DEPENDENCIES "@ARROW_SYSTEM_DEPENDENCIES@")
set(ARROW_BUNDLED_STATIC_LIBS "@ARROW_BUNDLED_STATIC_LIBS@")
+set(ARROW_STATIC_INSTALL_INTERFACE_LIBS "@ARROW_STATIC_INSTALL_INTERFACE_LIBS@")
include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
@@ -79,14 +80,9 @@ if(NOT (TARGET arrow_shared OR TARGET arrow_static))
PROPERTIES
IMPORTED_LOCATION
"${arrow_lib_dir}/${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}"
+ INTERFACE_LINK_LIBRARIES
+ "${ARROW_STATIC_INSTALL_INTERFACE_LIBS}"
)
-
- get_property(arrow_static_interface_link_libraries
- TARGET arrow_static
- PROPERTY INTERFACE_LINK_LIBRARIES)
- set_target_properties(
- arrow_static PROPERTIES INTERFACE_LINK_LIBRARIES
- "${arrow_static_interface_link_libraries};arrow_bundled_dependencies")
endif()
endif()
endif()
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index 5736c557b..66a24fbfc 100644
--- a/cpp/src/arrow/CMakeLists.txt
+++ b/cpp/src/arrow/CMakeLists.txt
@@ -526,6 +526,9 @@ endif()
if(ARROW_BUILD_BUNDLED_DEPENDENCIES)
string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies")
+ set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL "arrow_bundled_dependencies")
+else()
+ set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})
endif()
# Need -latomic on Raspbian.
# See also: https://issues.apache.org/jira/browse/ARROW-12860
@@ -557,7 +560,7 @@ add_arrow_lib(arrow
SHARED_INSTALL_INTERFACE_LIBS
${ARROW_SHARED_INSTALL_INTERFACE_LIBS}
STATIC_INSTALL_INTERFACE_LIBS
- ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})
+ ${ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL})
add_dependencies(arrow ${ARROW_LIBRARIES})
There was a problem hiding this comment.
Can we create a one-linear(?) to collect this list like https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L2659-L2667 ?
It may be better that we have a shell script that generates the list and include it from this file:
# cpp/build-support/list-grpc-absl-libraries.sh
...
echo "set(GRPC_GPR_ABSL_LIBRARIES absl::...)"$ cpp/build-support/list-grpc-absl-libraries.sh > cpp/cmake_modules/gRPCVariables.cmakediff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 66d04acae..36565fdbf 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -3475,14 +3475,7 @@ macro(build_grpc)
PROPERTIES IMPORTED_LOCATION "${GRPC_STATIC_LIBRARY_UPB}"
INTERFACE_INCLUDE_DIRECTORIES "${GRPC_INCLUDE_DIR}")
- set(GRPC_GPR_ABSL_LIBRARIES
- absl::base
- absl::statusor
- absl::status
- absl::cord
- absl::strings
- absl::synchronization
- absl::time)
+ include(gRPCVariables)
add_library(gRPC::gpr STATIC IMPORTED)
set_target_properties(gRPC::gpr
PROPERTIES IMPORTED_LOCATION "${GRPC_STATIC_LIBRARY_GPR}"
rok
force-pushed
the
ARROW-14708
branch
3 times, most recently
from
a87ae67 to
7d55e3c
Compare
|
Thanks for the feedback @kou and @lidavidm! I've moved things around as suggested. My cmake skills are lacking. I've also added a shell script to extract the |
rok
force-pushed
the
ARROW-14708
branch
3 times, most recently
from
1c9e45d to
9c13c18
Compare
|
I'm getting: When running: I'll continue tomorrow but would appreciate pointers :) |
|
Just a thought, maybe the |
|
Ah, it seems that we also need to specify diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 38c35d7e7..b88768790 100644
--- a/cpp/cmake_modules/BuildUtils.cmake
+++ b/cpp/cmake_modules/BuildUtils.cmake
@@ -240,7 +240,9 @@ function(ADD_ARROW_LIB LIB_NAME)
DEPENDENCIES
SHARED_INSTALL_INTERFACE_LIBS
STATIC_INSTALL_INTERFACE_LIBS
- OUTPUT_PATH)
+ OUTPUT_PATH
+ SHARED_INSTALL_EXTRA_TARGETS
+ STATIC_INSTALL_EXTRA_TARGETS)
cmake_parse_arguments(ARG
"${options}"
"${one_value_args}"
@@ -396,7 +398,7 @@ function(ADD_ARROW_LIB LIB_NAME)
"${_lib_install_name}")
endif()
- install(TARGETS ${LIB_NAME}_shared ${INSTALL_IS_OPTIONAL}
+ install(TARGETS ${LIB_NAME}_shared ${ARG_SHARED_INSTALL_EXTRA_TARGETS} ${INSTALL_IS_OPTIONAL}
EXPORT ${LIB_NAME}_targets
RUNTIME DESTINATION ${RUNTIME_INSTALL_DIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
@@ -451,7 +453,7 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
+ install(TARGETS ${LIB_NAME}_static ${ARG_STATIC_INSTALL_EXTRA_TARGETS} ${INSTALL_IS_OPTIONAL}
EXPORT ${LIB_NAME}_targets
RUNTIME DESTINATION ${RUNTIME_INSTALL_DIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
@@ -464,7 +466,7 @@ function(ADD_ARROW_LIB LIB_NAME)
arrow_install_cmake_find_module("${ARG_CMAKE_PACKAGE_NAME}")
set(TARGETS_CMAKE "${ARG_CMAKE_PACKAGE_NAME}Targets.cmake")
- install(EXPORT ${LIB_NAME}_targets
+ install(EXPORT ${LIB_NAME}_targets ${ARG_CMAKE_
FILE "${TARGETS_CMAKE}"
DESTINATION "${ARROW_CMAKE_INSTALL_DIR}")
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index 5736c557b..07520d919 100644
--- a/cpp/src/arrow/CMakeLists.txt
+++ b/cpp/src/arrow/CMakeLists.txt
@@ -557,7 +557,9 @@ add_arrow_lib(arrow
SHARED_INSTALL_INTERFACE_LIBS
${ARROW_SHARED_INSTALL_INTERFACE_LIBS}
STATIC_INSTALL_INTERFACE_LIBS
- ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})
+ ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}
+ STATIC_EXTRA_TARGETS
+ arrow_bundled_dependencies)
add_dependencies(arrow ${ARROW_LIBRARIES})
Or we can add Do you want me to take this up? |
|
@kou thanks for the pointers! -Wall -fno-semantic-interposition -msse4.2
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowTargets.cmake".
Call Stack (most recent call first):
src/arrow/CMakeLists.txt:539 (add_arrow_lib)
-- Creating bundled static library target arrow_bundled_dependencies at /home/rok/Documents/repos/arrow/cpp/build/release/libarrow_bundled_dependencies.a
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowTestingTargets.cmake".
Call Stack (most recent call first):
src/arrow/CMakeLists.txt:601 (add_arrow_lib)
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowDatasetTargets.cmake".
Call Stack (most recent call first):
src/arrow/dataset/CMakeLists.txt:51 (add_arrow_lib)
-- Found Python3: /home/rok/miniconda3/envs/pyarrow-dev/bin/python3.7 (found version "3.7.12") found components: Interpreter Development NumPy Development.Module Development.Embed
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowPythonTargets.cmake".
Call Stack (most recent call first):
src/arrow/python/CMakeLists.txt:70 (add_arrow_lib)
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ParquetTargets.cmake".
Call Stack (most recent call first):
src/parquet/CMakeLists.txt:240 (add_arrow_lib) |
|
@github-actions crossbow submit -g nightly |
|
Revision: aed4157ff68fcde5f16a472cd689640dc624f588 Submitted crossbow builds: ursacomputing/crossbow @ actions-1422
|
rok
force-pushed
the
ARROW-14708
branch
2 times, most recently
from
cc1b7a1 to
85c9208
Compare
rok
force-pushed
the
ARROW-14708
branch
2 times, most recently
from
f632d02 to
89bd9b4
Compare
|
@lidavidm Implemented your suggestions and added a workaround for the abseil issue on macOS. |
cpp/cmake_modules/BuildUtils.cmake
Outdated
There was a problem hiding this comment.
I'm still confused here, now the arguments were added back but we don't use them?
There was a problem hiding this comment.
This won't work on non-Apple platforms will it?
There was a problem hiding this comment.
Do we need this branch at all? Presumably absl::time's deps are correct on non-Apple platforms and we don't need to overwrite them?
There was a problem hiding this comment.
Sorry, that shouldn't be there.
There was a problem hiding this comment.
I'm not sure about absl::time's deps on non-Apple so leaving that branch as is.
cpp/examples/arrow/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Shouldn't this end with -Wl,--no-force-load or something?
Related, does it take -force-load or --force-load?
There was a problem hiding this comment.
So all_load/noall_load currently won't work due to symbol duplication so force-load is the only option and there appears to be no noforce-load option.
More here.
cpp/src/arrow/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Is this back to NOT? I feel like it flip-flops on every commit…was there a rebase error or something? It might be easier to squash current commits if needed
There was a problem hiding this comment.
Ugh, sorry. I was switching back and forth between linux and mac to fix the mac issue and probably caused this. I'll squash later today.
rok
force-pushed
the
ARROW-14708
branch
2 times, most recently
from
27e3cd0 to
f525cd6
Compare
|
@lidavidm thanks for your patience. I've pushed another change with your suggestions. |
| STATIC_INSTALL_INTERFACE_LIBS | ||
| OUTPUT_PATH) |
There was a problem hiding this comment.
Hmm, did you mean to remove this parameter?
cpp/src/arrow/ArrowConfig.cmake.in
Outdated
| ) | ||
|
|
||
| INTERFACE_LINK_LIBRARIES | ||
| ${ARROW_STATIC_INSTALL_INTERFACE_LIBS} |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Thanks! Things work for me now.
|
Sorry for the rebasing mess @lidavidm ! :) |
| endif() | ||
|
|
||
| if(ARROW_BUILD_BUNDLED_DEPENDENCIES) | ||
| string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies") |
cpp/src/arrow/CMakeLists.txt
Outdated
| ${ARROW_SHARED_INSTALL_INTERFACE_LIBS} | ||
| STATIC_INSTALL_INTERFACE_LIBS | ||
| ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}) | ||
| STATIC_INSTALL_INTERFACE_LIBS) |
There was a problem hiding this comment.
Can we remove STATIC_INSTALL_INTERFACE_LIBS?
cpp/src/arrow/CMakeLists.txt
Outdated
|
|
||
| if(ARROW_BUILD_BUNDLED_DEPENDENCIES) | ||
| string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies") | ||
| set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}) |
There was a problem hiding this comment.
Why do we need _REAL alias?
Is there any problem if we use ARROW_STATIC_INSTALL_INTERFACE_LIBS directly?
There was a problem hiding this comment.
Removing _REAL. It works for me locally, waiting or tests.
|
@kou thank you for the review! I've pushed suggested changes. |
|
Benchmark runs are scheduled for baseline = 5772d65 and contender = 2fea818. 2fea818 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
5 participants
No description provided.